Skip to content

Conversation

GuillaumeGomez
Copy link
Member

Before:

screen shot 2017-02-19 at 00 30 51

After:

screen shot 2017-02-19 at 00 30 39

cc @SergioBenitez

r? @rust-lang/docs

@frewsxcv
Copy link
Contributor

LGTM. Gonna hold off on doing an r+ to see if others have feedback.

@SergioBenitez
Copy link
Contributor

Definitely nicer! My only remaining concern is that long definitions still look quite bad. As an example, see ContentType::JSON. If the definition remains on the same line as the name of the constant, the rendering won't look great.

To ameliorate this, I propose that we adopt one of the following:

  1. Hide the definition entirely on the main page. Clicking on the constant's name leads to a page that shows the full definition as well as the documentation.
  2. Hide the definition by default but allow users to click an "expand" button to see the definition.
  3. Perhaps in conjunction with 2, move the definition to the body of the documentation, where the docstring is shown. Place the definition inside of a shaded pre section.

@GuillaumeGomez
Copy link
Member Author

I feel a bit off for hiding type and value information by default. In your case, the indent will allow to make the reading easier (which is helped by the new style).

@steveklabnik
Copy link
Contributor

Seems good to me too.

@QuietMisdreavus
Copy link
Contributor

Not quite a fan, but probably better than nothing. I like @SergioBenitez's suggestion 3, though I'm not sure how rustdoc receives the values of associated consts. It might take some finesse (read: duplicate code) to get it to render through the same pretty-print as a struct's definition.

@GuillaumeGomez
Copy link
Member Author

I just thought that moving associated consts in a "category" of their own might be problematic in case this value is only for certain implementation (I think about type parameters specialization).

@SergioBenitez
Copy link
Contributor

That's not what I'm proposing, @GuillaumeGomez. Keep the const documentation where it is, relatively, but make it look like:

const Other: usize

const Other: usize = 12

This is the documentation for this item.

A larger example:

const Any: ContentType

const Any: ContentType = ContentType{ttype: UncasedAscii{string: Cow::Borrowed("*"),},
           subtype: UncasedAscii{string: Cow::Borrowed("*"),},
           params: None,}

ContentType representing any Content-Type (*/*).

You can play with the formatting and optionally hide the definition by default.

@GuillaumeGomez
Copy link
Member Author

Ok, sorry the misunderstanding! This way seems to be quite good indeed. I'll give it a try.

@GuillaumeGomez
Copy link
Member Author

Ok, I updated as suggested:

Hidden:

screen shot 2017-02-26 at 18 33 12

Not hidden:

screen shot 2017-02-26 at 18 33 14

By default they're visible.

@SergioBenitez
Copy link
Contributor

@GuillaumeGomez I think we still want to know the type in the non-hidden version, right?

@frewsxcv
Copy link
Contributor

Looking great @GuillaumeGomez, nice job :)

@SergioBenitez
Copy link
Contributor

Actually, if the definition is visible by default, maybe it's okay to not include the type in the hidden version?

@GuillaumeGomez
Copy link
Member Author

I'll need to add tests as well.

@GuillaumeGomez
Copy link
Member Author

So, I added all missing types in the "correct" output and added tests (and filed an ICE, so nice of me).

@steveklabnik
Copy link
Contributor

@SergioBenitez what do you think?

@frewsxcv
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 28, 2017

📌 Commit bd704ba has been approved by frewsxcv

@bors
Copy link
Collaborator

bors commented Feb 28, 2017

⌛ Testing commit bd704ba with merge 9b19645...

@frewsxcv
Copy link
Contributor

@bors retry

@bors
Copy link
Collaborator

bors commented Feb 28, 2017

⌛ Testing commit bd704ba with merge f7cc0c6...

@alexcrichton
Copy link
Member

@bors: retry

Looks like travis missed this PR...

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 1, 2017
…=frewsxcv

Improve associated constant rendering in rustdoc

Before:

<img width="1440" alt="screen shot 2017-02-19 at 00 30 51" src="https://cloud.githubusercontent.com/assets/3050060/23097697/caeed80e-f63a-11e6-98c2-5d27e4efd76d.png">

After:

<img width="1440" alt="screen shot 2017-02-19 at 00 30 39" src="https://cloud.githubusercontent.com/assets/3050060/23097698/cfb4874e-f63a-11e6-80cf-ffbf5c5c6162.png">

cc @SergioBenitez

r? @rust-lang/docs
bors added a commit that referenced this pull request Mar 1, 2017
Rollup of 6 pull requests

- Successful merges: #39419, #39936, #39944, #39960, #40028, #40128
- Failed merges:
bors added a commit that referenced this pull request Mar 1, 2017
Rollup of 6 pull requests

- Successful merges: #39419, #39936, #39944, #39960, #40028, #40128
- Failed merges:
@bors bors merged commit bd704ba into rust-lang:master Mar 1, 2017
@GuillaumeGomez GuillaumeGomez deleted the associated-consts branch March 1, 2017 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants